Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Less unsafeCoerce #322

Merged
merged 2 commits into from
Aug 5, 2019
Merged

Conversation

treeowl
Copy link
Contributor

@treeowl treeowl commented Jul 9, 2019

Get rid of most unnecessary uses of unsafeCoerce. Several of the ones that remain are removed by #320.

@treeowl treeowl mentioned this pull request Jul 9, 2019
@treeowl
Copy link
Contributor Author

treeowl commented Jul 9, 2019

Big question: how should I name the RIO module? I don't want it getting mixed up with the one from rio, since it's pretty much internal. I don't care myself.

@treeowl
Copy link
Contributor Author

treeowl commented Jul 9, 2019

@Ericson2314 I was quite unable to get anything good out of the manual case on Coercion idea. For whatever reason, I couldn't get GHC to infer the right coercion flow. But I was able to dramatically reduce the type annotations I was using before.

@treeowl treeowl force-pushed the less-unsafeCoerce2 branch 2 times, most recently from 017ac4c to cde1b93 Compare July 9, 2019 19:34
@Ericson2314
Copy link
Member

Ericson2314 commented Jul 9, 2019

@treeowl I think you forget to add the RIO module. OTOH I think there might be another way around the RIO stuff through transitive coercions? [coNth is restricted to disallow Coercible (NewType a) (NewType b) => a ~ b for just this reason of there being ways to get around the nominal role to create the the LHS coercion in the first place.]

@treeowl
Copy link
Contributor Author

treeowl commented Jul 9, 2019

@Ericson2314 Whoops, fixed that. The only way I see around RIO is to do something like DynamicS x p = Dynamic x (Behavior x (PatchTarget p)) p. That might do the trick, but it doesn't exactly improve clarity.

@Ericson2314
Copy link
Member

@treeowl Yeah and also if other roles depend on BehaviorM their's no opportunity to provide your own coercion to prove a non-nominal role.

@cgibbard
Copy link
Collaborator

cgibbard commented Jul 9, 2019 via email

@Ericson2314
Copy link
Member

@cgibbard see I wrote that above :) @treeowl I think you meant to push but didnt'?

@treeowl
Copy link
Contributor Author

treeowl commented Jul 9, 2019

Oh crud. Yeah, I forgot to push. Actually fixed now, I believe!

@ryantrinkle
Copy link
Member

To avoid confusion with https://github.com/commercialhaskell/rio/blob/10d0bd8c6b03035504c5c086dc90ad85cdb0b914/rio/src/RIO/Prelude/RIO.hs#L38, I think it would be better for us to choose an uglier name for our reader-over-IO. In particular, our goals are pretty different: RIO is aimed at improving the elegance of user code, whereas this reader-over-IO is simply a hack around the role system's inability to express quite the right thing. Given that one can't be substituted for the other (RIO doesn't fix the role issue, and this thing doesn't help with any of RIO's goals), I think it would be better to use a different, uglier name. I'd propose ReaderT_IO or ReaderIO just so it's blatant what's going on. Ideally, if GHC improves the role system in the future, this would go away.

Remove most uses of `unsafeCoerce`. Most of the rest are taken
care of in my `mergeG` pull request.
@treeowl
Copy link
Contributor Author

treeowl commented Aug 5, 2019

@ryantrinkle I've renamed the module and type as you requested.

@oliver-batchelor oliver-batchelor merged commit 5e037c0 into reflex-frp:develop Aug 5, 2019
ali-abrar added a commit that referenced this pull request Aug 23, 2019
* 'release/0.6.2.3' of github.com:reflex-frp/reflex: (34 commits)
  v0.6.2.3 - Add an upper-bound to witherable
  Use align (a merge) instead of coincidence for <.> (#346)
  Allow witherable ==0.3.*
  Use haskell-ci regenerate
  Update .travis.yml
  Add freeze file to avoid text 1.2.4.0 duplicate instances  (#347)
  default.nix: Add missing profunctors dep
  Use xenial and manually install node 8 for ghcjs
  Fix merges (#340)
  contributing: add note on dependency ver bounds
  travis-ci: Add ghcjs to allowed failures list bc of travis time limit
  Less unsafeCoerce (#322)
  travis: disable benchmarks
  changelog: document v0.6.2.2
  ci: Fix travis config
  cabal: Avoid hlint 2.1.*
  cabal: Limit witherable version
  ci: Remove cabal.project.local before building
  hlint: More linting
  cabal: remove some version bounds
  ...
matthewbauer added a commit that referenced this pull request Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants